-
Notifications
You must be signed in to change notification settings - Fork 71
Create TextNode component #3172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 449e6da The changes in this PR will be included in the next version bump. This PR includes changesets to release 65 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new TextNode
component to the typography package that conditionally wraps string/number children in a specified HTML element or React component, while passing through React nodes unchanged.
Key changes:
- Creates a new utility component for handling mixed string/ReactNode children
- Adds comprehensive test coverage for various input types and edge cases
- Documents the component's behavior through changeset and inline comments
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/typography/src/index.ts |
Exports the new TextNode component |
packages/typography/src/TextNode/TextNode.tsx |
Implements the TextNode component with conditional rendering logic |
packages/typography/src/TextNode/TestNode.spec.tsx |
Comprehensive test suite covering string, ReactNode, and edge cases |
.changeset/typography-text-node.md |
Documents the new component for release notes |
@@ -0,0 +1,128 @@ | |||
import React, { PropsWithChildren } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename 'TestNode.spec.tsx' contains a typo. It should be 'TextNode.spec.tsx' to match the component name.
Copilot uses AI. Check for mistakes.
Useful when rendering `children` props that can be any react node | ||
|
||
```tsx | ||
<TextNode as={h1}>Hello!</TextNode> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component reference 'h1' should be a string 'h1' for HTML elements. This will cause a runtime error as h1 is not defined.
<TextNode as={h1}>Hello!</TextNode> | |
<TextNode as="h1">Hello!</TextNode> |
Copilot uses AI. Check for mistakes.
Size Change: -7.72 kB (-0.48%) Total Size: 1.59 MB
ℹ️ View Unchanged
|
export const TextNode = ({ | ||
children, | ||
as, | ||
}: PropsWithChildren<{ as?: PolymorphicAs }>) => { | ||
return typeof children === 'string' || typeof children === 'number' ? ( | ||
<Polymorph as={as}>{children}</Polymorph> | ||
) : ( | ||
children | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what cases will this be used? Is it intended to be a wrapper that replaces logic elsewhere like in Description
? If so, I'm wondering if we should add an explicit wrapper <div>
around children
on L27 so attributes/props can be passed to the component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A use case for this could be Drawer
. We currently have this:
leafygreen-ui/packages/drawer/src/Drawer/Drawer.tsx
Lines 263 to 270 in bda06c5
<Body | |
as={typeof title === 'string' ? 'h2' : 'div'} | |
baseFontSize={BaseFontSize.Body2} | |
id={titleId} | |
className={titleStyles} | |
> | |
{title} | |
</Body> |
but that could be replaced with TextNode
like this:
const Wrapper = ({ children }: PropsWithChildren<{}>) => (
<Body
as={'h2'}
baseFontSize={BaseFontSize.Body2}
id={titleId}
className={titleStyles}
>
{title}
</Body>
);
<TextNode as={Wrapper}>{title}</TextNode>
However, in this case, the consumer will lose the id
attribute if using a ReactNode
so being able to pass that id
to the ReactNode
could be beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good example! Yes, it seems like being able to pass attributes/props to the TextNode
would be important
✍️ Proposed changes
'@leafygreen-ui/typography': minor
Adds
TextNode
component.Wraps a string in the provided
as
component,or renders the provided
ReactNode
.Useful when rendering
children
props that can be any react node